Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize code adjust #2700

Merged
merged 9 commits into from Jun 1, 2021
Merged

Optimize code adjust #2700

merged 9 commits into from Jun 1, 2021

Conversation

daheige
Copy link
Contributor

@daheige daheige commented Apr 22, 2021

1.setFormMap error of result
2.adjust code for TrySet
3.error export for type multipart.FileHeader
4.code style adjust
5.reflect code maping optimize

@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

Merging #2700 (7e656c6) into master (6703dea) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2700   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files          41       41           
  Lines        2074     2074           
=======================================
  Hits         2047     2047           
  Misses         15       15           
  Partials       12       12           
Impacted Files Coverage Δ
binding/header.go 100.00% <ø> (ø)
render/json.go 85.29% <ø> (ø)
binding/form_mapping.go 100.00% <100.00%> (ø)
binding/multipart_form_mapping.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6703dea...7e656c6. Read the comment docs.

var (
errUnknownType = errors.New("unknown type")

// ErrConvertMapStringSlice covert to map[string][]string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace covert to map[string][]string with can not covert to map[string][]string

Copy link
Contributor Author

@daheige daheige Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not what you said, this is the error returned, as a developer using gin can know the error of the underlying transformation, rather than what you said not to let others know, which is a bad design

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// ErrConvertMapStringSlice covert to map[string][]string
// ErrConvertToMapString can not convert to map[string]string

I don't know why the format can't be the same between ErrConvertMapStringSlice and ErrConvertToMapString?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that comment message.

errors.New("can not convert to map of strings")

comment message:

// can not convert to map[string]string

but

errors.New("can not convert to map slices of strings")

comment message:

// covert to map[string][]string

not

// can not convert to map[string][]string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, if there are two errors here, they should be separated. As for the comment information you said, you can modify it appropriately. For the function point here, I hope the caller knows why the error occurred, instead of returning as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that comment message.

errors.New("can not convert to map of strings")

comment message:

// can not convert to map[string]string

but

errors.New("can not convert to map slices of strings")

comment message:

// covert to map[string][]string

not

// can not convert to map[string][]string

You can change the notes here. Thank you very much

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to change the comment

-// ErrConvertMapStringSlice covert to map[string][]string
+// ErrConvertMapStringSlice can not covert to map[string][]string

@appleboy appleboy added this to the v1.8 milestone Apr 28, 2021
ErrConvertMapStringSlice = errors.New("can not convert to map slices of strings")

// ErrConvertToMapString can not convert to map[string]string
ErrConvertToMapString = errors.New("can not convert to map of strings")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not output var

Copy link
Contributor Author

@daheige daheige Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is an error in the conversion, it should be made known to the caller, not hidden in the current gin framework. I don't think your gin team has done a very good job of handling the error, and the way it is handled is not very friendly to the caller. I think there is something wrong with the design. Maybe more Go developers feel the same way.

@appleboy
Copy link
Member

Why created the other two PR #2709 and #2708 ?

@daheige
Copy link
Contributor Author

daheige commented Apr 29, 2021 via email

@daheige
Copy link
Contributor Author

daheige commented Apr 29, 2021 via email

@daheige
Copy link
Contributor Author

daheige commented Apr 29, 2021 via email

@daheige
Copy link
Contributor Author

daheige commented Apr 29, 2021 via email

@appleboy appleboy requested a review from thinkerou April 30, 2021 23:13
Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@thinkerou thinkerou merged commit 97a32b1 into gin-gonic:master Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants